fix: allow same-name staged attachments#1950
Open
Ap4sh wants to merge 1 commit into
Open
Conversation
Ap4sh
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR migrates staged-attachment identity and removal semantics from fileName-based to a stable stagedAttachmentId, preventing collisions when multiple attachments share the same filename.
Changes:
- Add
stagedAttachmentIdtoStagedAttachmentTypeand generate ids when staging files/previews. - Update staged-attachments reducer to de-dupe and remove by
stagedAttachmentIdinstead offileName, and tighten voice-message staging rules. - Add unit tests covering de-dupe/removal behavior and voice-message constraints.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ts/util/attachment/attachmentsUtil.ts | Updates imported staged-attachment type definition to omit stagedAttachmentId. |
| ts/state/ducks/stagedAttachments.ts | Switches uniq/removal logic to use stagedAttachmentId and refines voice-message gating. |
| ts/components/conversation/composition/CompositionBox.tsx | Changes audio voice-note staged attachment shape/type. |
| ts/components/conversation/StagedAttachmentList.tsx | Removes staged attachments by stagedAttachmentId and uses it as a React key fallback. |
| ts/components/conversation/SessionConversation.tsx | Assigns stagedAttachmentId when creating staged attachments and previews. |
| ts/test/session/unit/staged_attachments/StagedAttachments_test.ts | Adds unit tests for id-based staging/removal and voice-message constraints. |
Comments suppressed due to low confidence (1)
ts/util/attachment/attachmentsUtil.ts:1
- The reducer and UI now treat
stagedAttachmentIdas the primary identity (de-dupe, removal, React keys). OmittingstagedAttachmentIdfromStagedAttachmentImportedTypemakes it easy to construct “staged-like” objects without ids (as seen inCompositionBox), which breaks those invariants. Prefer keepingstagedAttachmentIdon imported staged attachments, or enforce a single conversion step that always generates/assignsstagedAttachmentIdbefore such objects can reach the staged-attachments state/UI.
/* eslint-disable max-len */
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+811
to
819
| const audioAttachment: StagedAttachmentImportedType = { | ||
| contentType: MIME.AUDIO_MP3, | ||
| size: savedAudioFile.size, | ||
| fileSize: null, | ||
| screenshot: null, | ||
| fileName: 'session-audio-message', | ||
| thumbnail: null, | ||
| url: '', | ||
| isVoiceMessage: true, | ||
| path: savedAudioFile.path, | ||
| }; |
Author
There was a problem hiding this comment.
This path does not go through staged attachment state
sendVoiceMessage builds a StagedAttachmentImportedType and sends it directly through sendMessage, while stagedAttachmentId is only required on StagedAttachmentType before getFileAndStoreLocally
Keeping it omitted from StagedAttachmentImportedType is intentional so the local staging id does not leak into outgoing attachment payloads
Comment on lines
+38
to
45
| if ( | ||
| (hasNewVoiceMessage && | ||
| (currentStagedAttachments.length > 0 || newAttachments.length > 1)) || | ||
| (hasCurrentVoiceMessage && newAttachments.length > 0) | ||
| ) { | ||
| window?.log?.warn('A voice note cannot be sent with other attachments'); | ||
| return state; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First time contributor checklist:
Contributor checklist:
devbranchDescription
Fixes #241
This fixes staged attachment handling when multiple selected files have the same filename
Previously staged attachments were deduped and removed by
fileName, so adding two files namedimage.jpgonly kept one item. Removing by filename also could not target one specific same-name attachmentThis PR adds a local
stagedAttachmentIdfor staged attachments and uses that id for rendering, dedupe, and single-item removal. The id stays local to staged attachments and is not included in outgoing attachment payloadsTesting
corepack pnpm dedupe --checktsc, Babel compile, Sass, SVG, and workerscorepack pnpm exec prettier --check "*.{css,js,json,scss,ts,tsx}" "./**/*.{css,js,json,scss,ts,tsx}"corepack pnpm exec eslint --cache .912 passing